Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: fix passthru.tests #247520

Merged
merged 2 commits into from
Aug 8, 2023
Merged

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Aug 6, 2023

Description of changes

#191540 indirectly broke kernel
passthru.tests; calling the testsForLinuxPackages and testsForKernel functions
with some args intended for some other exposed test-internal function.

Organise the passed-through functions under passthru to prevent this from
happening.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Atemu added 2 commits August 6, 2023 15:40
discoverTests tries to discover some sort of internal function and tries to call
it with the arguments for that internal function. This poses an issue when you
want to expose some other functions (i.e. a parameterisation for a test) in
nixosTests.

This commit allows a test to pass through arbitrary values via `.passthru`
without them having discovery applied to them; including functions.
NixOS#191540 indirectly broke kernel
passthru.tests; calling the testsForLinuxPackages and testsForKernel functions
with some args intended for some other exposed test-internal function.

Organise the passed-through functions under `passthru` to prevent this from
happening.
@Atemu Atemu added 6.topic: kernel The Linux kernel 6.topic: testing Tooling for automated testing of packages and modules labels Aug 6, 2023
@Atemu Atemu requested a review from roberth August 6, 2023 13:51
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Aug 6, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Aug 6, 2023
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what to think. I feel like this complication of the all-tests.nix code is going to be the final nail in the coffin. Maybe that's ok if it solves the problem at hand and we burn it later.

@@ -21,7 +21,7 @@ let
if isAttrs val
then
if hasAttr "test" val then callTest val
else mapAttrs (n: s: discoverTests s) val
else mapAttrs (n: s: if n == "passthru" then s else discoverTests s) val
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc this code is used by both release.nix and nixosTests, and it has to deal with all tests, which are still written in a number of styles.
So this is hard to review; can't really tell the implications without a lot of manual testing.

In my opinion release.nix should be rewritten to follow the more easily understood structure of nixosTests, but until that's done, we need to consider that both code paths used this ill-conceived semi-generic code.

All that aside, we should really try to avoid the proliferation of the term "passthru". Unfortunately it's the mechanism we have to use with mkDerivation, but it is by itself meaningless, and its presence in the package attribute set is an implementation detail. So if we're going to accept a fix for the problem it would be great for it not to use the passthru attribute.

As for rewriting release.nix, ideally we'd have something like #176557 which has a nice and general solution for this particular problem of overriding a test to use a specific package. But the actual implementation was a bit messy, which is why I think it should be rewritten. It would probably still resemble the documented example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this isn't a great solution. It's really more of a workaround and I don't like adding special cases like this but I don't really see another way without refactoring the entire tests infrastructure which I don't feel comfortable doing and even you doing it would take quite some time (see #176557).

By using "passthru" as the special attribute's name, I hope to make it somewhat clear that it's intended for passing through values rather than exposing tests. Do you have a better name for it?

All this should change is that anything organised under a passthru isn't discovered as a test. My hope is that no Nixpkgs contributor, knowing the meaning of passthru in mkDerivation, would organise an actual test under "passthru", only values for, well, passing through.

@roberth roberth merged commit bc9d2d6 into NixOS:master Aug 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

Successfully created backport PR for release-23.05:

@Atemu Atemu deleted the fix/kernel-passthru.tests branch August 9, 2023 07:18
@Atemu
Copy link
Member Author

Atemu commented Aug 9, 2023

Thanks!

roberth added a commit to hercules-ci/nixpkgs that referenced this pull request Aug 9, 2023
The .overrideAttrs part is ok now, but a "passthru' workaround is
necessary now. See NixOS#247520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: kernel The Linux kernel 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: testing Tooling for automated testing of packages and modules 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants